Skip to content

fix(codex): retain Spark quota on partial header updates - #4008

Closed
cb8010d6 wants to merge 1 commit into
lidge-jun:devfrom
cb8010d6:fix/preserve-spark-quota-windows
Closed

fix(codex): retain Spark quota on partial header updates#4008
cb8010d6 wants to merge 1 commit into
lidge-jun:devfrom
cb8010d6:fix/preserve-spark-quota-windows

Conversation

@cb8010d6

@cb8010d6 cb8010d6 commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes #4007.

After refreshing Spark quota, an ordinary Codex response-header update drops the stored Spark weekly window because those headers omit customWindows. Retain the previous custom windows when the update omits them. Explicit replacements (including an empty array) and account-cache clearing keep their existing behavior.

The runtime change is limited to the partial-snapshot merge; it does not change quota parsing, account selection, authentication, or display settings. Regression coverage exercises the actual WHAM parse -> store -> response-header update sequence, replacement with zero usage, explicit clearing, and account-cache clearing.

Verification

  • Reproduced before the fix: the new WHAM/header regression failed because customWindows became undefined.
  • After the fix: 184 tests passed across codex-quota-parser-parity, codex-spark-visibility, main-quota-evidence-validation, and main-quota-provenance.
  • bun run typecheck: passed.
  • bun run privacy:scan: passed.
  • git diff --check: passed.
  • bun run test:changed: selected 821 of 1,145 files and exceeded the runner's 900-second limit (exit 124). The run also reported individual test timeouts, including API-key management tests. Those failures have not been attributed or compared against an unmodified base; this is not an all-green result. The explicitly invoked repository-wide suite has not been run, and this PR remains draft.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed. This restores existing partial-update behavior without changing configuration or workflows.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. No credentials or real account data are included.

Review readiness checklist

  • All CI tests are green on my local testing.
  • I pushed my PR to the latest dev commit.
  • I resolved all correct Codex and CodeRabbit findings.
  • My PR is ready for review.

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

  • All CI tests are green on my local testing.

  • I pushed my PR to the latest dev commit.

  • I resolved all correct Codex and CodeRabbit findings.

  • My PR is ready for review.

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@coderabbitai

coderabbitai Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: ceba2ed3-418c-44f1-a81c-e6f967704abf

📥 Commits

Reviewing files that changed from the base of the PR and between 74f62f9 and 522e438.

📒 Files selected for processing (2)
  • src/codex/quota.ts
  • tests/codex-integration/codex-quota-parser-parity.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The quota cache now preserves previously stored custom windows when partial updates omit them. Explicit lists still replace the stored value. Regression tests cover preservation, replacement, empty lists, and account cache clearing.

Changes

Quota preservation

Layer / File(s) Summary
Merge behavior and regression coverage
src/codex/quota.ts, tests/codex-integration/codex-quota-parser-parity.test.ts
mergeAccountQuota retains customWindows when an incoming snapshot omits the field. Explicit values, including an empty list, replace the stored value. Tests cover partial header updates, replacements, and account cache clearing.

Priority: ➖ Normal — Impact reflects medium issue severity.

Estimated code review effort: 2 (Simple) | ~10 minutes

Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 522e4

Partial quota-header updates now retain existing Spark custom windows, while explicit replacements, clearing, and account-cache resets continue to work. No current merge-blocking risk remains.

Suggested reviewers: lidge-jun

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes in src/codex/quota.ts retain existing customWindows when an incoming update omits the field, while explicit lists and explicit empty lists replace the stored value. The regression test…
Out of Scope Changes check ✅ Passed The code change and regression tests are directly related to the quota-loss defect in [#4007]. The changes do not introduce unrelated behavior, public API changes, or unrelated files.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: retaining Spark quota during partial response-header updates.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the bug Something isn't working label Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

⏳ DRAFT

  • review readiness checklist open (0/4 boxes ticked).

What to do

  • Tick all four boxes in the PR description once you're done (currently 0/4).

Review readiness checklist

  • ⬜ All CI tests are green on my local testing.
  • ⬜ I pushed my PR to the latest dev commit.
  • ⬜ I resolved all correct Codex and CodeRabbit findings.
  • ⬜ My PR is ready for review.

0/4 boxes ticked.

This PR stays in draft until every box above is ticked.

Hygiene

Deterministic PR hygiene checks passed.

@cb8010d6

cb8010d6 commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

Please review the partial quota merge fix and the regression sequence on the current head. The test reproduced missing Spark custom windows before the fix. Explicit replacement, empty-list clearing, and account-cache clearing are covered. This remains draft while broader verification is pending.

@coderabbitai

coderabbitai Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '--- PR diff stat ---'
git diff --stat HEAD^ HEAD
echo
echo '--- Changed files ---'
git diff --name-only HEAD^ HEAD
echo
echo '--- Diff ---'
git diff --unified=80 HEAD^ HEAD
echo
echo '--- Candidate quota/cache files ---'
fd -t f . | rg -i '(codex|quota|spark|account|cache)'

Length of output: 87442


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '--- quota module outline ---'
ast-grep outline src/codex/quota.ts --items all --view expanded
echo
echo '--- merge helpers and public update/clear APIs ---'
rg -n -A35 -B12 \
  'function (mergeAccountQuota|snapshotHasCustom|snapshotHasUsage)|export function (setAccountQuotaFromParsed|applyAccountQuotaFromUpstreamHeaders|clearAccountQuota|getAccountQuota)' \
  src/codex/quota.ts
echo
echo '--- quota types ---'
ast-grep outline src/codex/quota-types.ts --items all --view expanded
cat -n src/codex/quota-types.ts
echo
echo '--- merge call sites and custom-window consumers ---'
rg -n -A5 -B5 \
  '\bmergeAccountQuota\s*\(|\bcustomWindows\b' \
  src tests/codex-integration/codex-spark-visibility.test.ts tests/codex-integration/main-quota-provenance.test.ts tests/codex-integration/main-quota-evidence-validation.test.ts
echo
echo '--- focused test outline ---'
ast-grep outline tests/codex-integration/codex-quota-parser-parity.test.ts --items all --view expanded

Length of output: 78495


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '--- quota update, hydration, and WHAM parsing ---'
sed -n '492,625p' src/codex/quota.ts
sed -n '703,825p' src/codex/quota.ts
echo
echo '--- quota disk shape validation ---'
sed -n '540,611p' src/codex/quota.ts
echo
echo '--- changed regression tests with line numbers ---'
cat -n tests/codex-integration/codex-quota-parser-parity.test.ts | sed -n '1,82p'
echo
echo '--- existing Spark quota parse tests ---'
rg -n -A24 -B12 \
  'Spark|GPT-5\.3-Codex-Spark|additional_rate_limits|customWindows' \
  tests/codex-integration/codex-quota-parser-parity.test.ts \
  tests/codex-integration/codex-spark-visibility.test.ts
echo
echo '--- diff whitespace and worktree status ---'
git diff --check HEAD^ HEAD
git status --short

Length of output: 43674


@cb8010d6 Reviewed the current head of #4008. I found no blocking issues.

src/codex/quota.ts now preserves existing.customWindows when an incoming quota snapshot omits the field. snapshotHasCustom() uses customWindows !== undefined, so an explicit customWindows: [] still replaces the stored list. This preserves the required distinction between a partial header update and an explicit clear.

The regression coverage in tests/codex-integration/codex-quota-parser-parity.test.ts is appropriate:

  • Lines 13-29 execute the production sequence: WHAM parse, store, then applyAccountQuotaFromUpstreamHeaders.
  • Lines 31-41 verify replacement and explicit empty-list clearing.
  • Lines 43-50 verify that clearAccountQuota(accountId) prevents retained windows from crossing the account-cache boundary.

The static review also confirmed that stored custom windows feed quota observation and capacity paths. Retaining them prevents the header update from removing usable Spark quota state.

git diff --check passed. I could not run the Bun test suite in this sandbox.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 74 / 80

이 PR은 지금 dev(HEAD 74f62f9c2, package.json 2.48.0, #3993 server-owned Always proactive presets / #3991 xAI string child-result / #3992 proxy V2 guidance wrapper 직후)에서 Spark 주간 할당량이 화면에서 사라지는 좁은 버그를 고칩니다. #4007이 말하는 흐름은 이렇습니다. 대시보드에서 Spark 표시(showCodexSparkQuota)를 켠 뒤 Codex 계정을 새로고침하면 parseUsageQuota가 WHAM의 additional_rate_limits에서 GPT-5.3-Codex-Spark 창을 읽어 customWindows에 넣습니다. 그다음 보통 응답이 x-codex-primary-used-percent 같은 표준 헤더만 실어 오면 applyAccountQuotaFromUpstreamHeadersparseUpstreamQuotaHeaderssetAccountQuotaFromParsedmergeAccountQuota로 들어갑니다. 헤더 파서는 모델별 창을 전혀 만들지 않는데, 현재 HEAD의 src/codex/quota.ts mergeAccountQuotasnapshotHasCustom(quota)일 때만 next.customWindows를 넣고, 그렇지 않으면 기존 값을 이어받지 않습니다. 같은 함수가 weekly/short/resetCredits는 “없으면 이전 값 유지”로 이미 짜여 있고, creditsOnly 분기도 customWindows를 지키며, updateAccountQuota도 기존 customWindows를 스프레드로 붙입니다. 즉 Spark만 partial merge에서 유독 끊기는 구멍입니다.

고치는 코드는 한 갈래입니다. if (snapshotHasCustom(quota)) next.customWindows = quota.customWindows; 뒤에 else if (existing?.customWindows !== undefined) next.customWindows = existing.customWindows;를 붙여, 필드가 빠진 업데이트는 이전 창을 남기고, customWindows: []처럼 명시적으로 온 값(빈 배열 포함)은 그대로 덮어씁니다. snapshotHasCustom!== undefined라서 빈 배열 치환과 캐시 clearAccountQuota 후 재기록은 기존과 같습니다. 회귀 테스트 세 개는 WHAM 저장 → 표준 헤더 갱신 → Spark 유지, 명시 치환/빈 배열, 계정 캐시 clear 후 미이월을 실제로 setAccountQuotaFromParsed / applyAccountQuotaFromUpstreamHeaders 경로로 밟습니다. types.ts/config.ts 분할에 걸려 무효화될 변경이 아니고, 인증·계정 선택·표시 설정·파서 자체는 건드리지 않습니다. 방향은 현재 dev의 account-pool/quota 열차(#3607 리셋 마커, #4002 수동 리셋 쿨다운 등과 같은 층)와 잘 맞고, 범위도 작아서 머지 가치가 큽니다.

라인 src/codex/quota.ts mergeAccountQuota customWindows else 분기 - 의도는 맞습니다. 다만 WHAM 새로고침 없이 헤더만 계속 오면 Spark 퍼센트/리셋이 마지막 WHAM 값에 고정됩니다. 표준 창은 매 응답마다 갱신되는데 Spark만 오래된 숫자로 남을 수 있습니다. 버그 티켓이 요구한 “생략≠삭제”에는 충분하지만, 오래된 Spark 바를 얼마나 허용할지는 제품 판단입니다.

경로 테스트 Spark quota survives partial header updates - 유지/치환/clear는 잘 덮습니다. 빠진 케이스는 (1) policyEvidence=true 메인 정책 병합에서도 같은 else가 도는지(함수 하나로 가니 아마 되지만 단언이 없음), (2) customWindows에 Spark가 아닌 다른 라벨만 있을 때 헤더 partial이 그걸도 유지하는지(코드상 유지됨), (3) showCodexSparkQuota === false일 때 src/codex/auth-api.ts가 표시만 걸러 주고 저장본은 남는 현재 계약이 깨지지 않는지입니다. 필수는 아니고 Ready 전에 한 줄 확인이면 됩니다.

경로 게이트 - PR이 아직 draft이고 Review readiness checklist가 비어 있습니다. resolve-pr / hygiene / CodeRabbit은 통과해 보이지만, 작성자 본문도 “import-connected tests 진행 중, repo-wide suite 미실행, draft 유지”라고 적혀 있습니다. Ready·CI 초록 전에는 머지 열차에 올리면 안 됩니다.

경로 인접 PR - #3973 / #3995 / #4002는 리셋 크레딧·쿨다운 쪽이고, 이 PR의 customWindows partial merge와 겹치지 않습니다. 서로 막지 말고 독립으로 가져가면 됩니다. #4007은 이 PR이 Closes로 닫으면 됩니다.

메인테이너의 판단이 필요한 지점

  • Spark 창을 WHAM 주기까지 stale로 두는 현재 절충을 그대로 둘지, 나중에 Spark 전용 재프로브를 넣을지
  • draft 체크리스트·로컬/CI 범위가 충분하다고 보면 Ready 후 바로 머지할지(범위상 maintainer-sponsored가 꼭 필요해 보이지는 않음 — OAuth 표면이 아니라 quota merge 한 줄)
  • 정책(policyEvidence) 경로에 대한 추가 단언을 Ready 조건으로 요구할지

너의 추천
머지 방향이 맞습니다. 작성자에게 checklist를 채우고 draft를 Ready로 올린 뒤, 메인테이너는 #4007과 함께 빠르게 랜딩하세요. 닫거나 리베이스할 이유는 없고, types/config 분할과도 무관합니다. Spark stale 허용만 한 줄로 이슈/PR에 남겨 두면 이후 프로브 작업과 헷갈리지 않습니다.

이 댓글은 grok-bot이 작성했습니다

lidge-jun added a commit that referenced this pull request Sep 8, 2026
…rage, history) (#4051)

* fix(codex): keep Spark five-hour quota model-scoped

Carried from #4018 (author cb8010d6) for maintainer-side CI and sequential integration.
Closes #4017

Co-authored-by: cb8010d6 <53855466+cb8010d6@users.noreply.github.com>

* fix(codex): retain Spark quota on partial header updates

Carried from #4008 (author cb8010d6) for maintainer-side CI and sequential integration.
Closes #4007

Co-authored-by: cb8010d6 <53855466+cb8010d6@users.noreply.github.com>

* fix(codex): invalidate app-server observations at catalog boundaries

Carried from #3981 (author yansigit) for maintainer-side CI and sequential integration.

Co-authored-by: yansigit <44089734+yansigit@users.noreply.github.com>

* fix(web-search): stop inactivity timing after terminal events

Carried from #3979 (author yansigit) for maintainer-side CI and sequential integration.

Co-authored-by: yansigit <44089734+yansigit@users.noreply.github.com>

* fix(responses): strip Muse web_search fields on direct Meta

Carried from #3964 (author ildunari) for maintainer-side CI and sequential integration.

Co-authored-by: ildunari <95185577+ildunari@users.noreply.github.com>

* fix: preserve combo capabilities and skip referenced archives

Carried from #3863 (author x3M3x) for maintainer-side CI and sequential integration.

Co-authored-by: x3M3x <98298256+x3M3x@users.noreply.github.com>

* fix(codex): recover ocx1-compacted threads for native replay

Carried from #3920 (author cb8010d6) for maintainer-side CI and sequential integration.
Closes #3916

Co-authored-by: cb8010d6 <53855466+cb8010d6@users.noreply.github.com>

---------

Co-authored-by: t <a@b.com>
Co-authored-by: cb8010d6 <53855466+cb8010d6@users.noreply.github.com>
Co-authored-by: yansigit <44089734+yansigit@users.noreply.github.com>
Co-authored-by: ildunari <95185577+ildunari@users.noreply.github.com>
Co-authored-by: x3M3x <98298256+x3M3x@users.noreply.github.com>
@lidge-jun

Copy link
Copy Markdown
Owner

Landed on dev in 9587750 via #4051, which carried seven reviewed contributor fixes as one sequential integration branch. Your commit is preserved in the squash body with a Co-authored-by trailer, so the contribution stays attributed to you.

Verification at the merged head 1e32dee6e: Cross-platform CI run 34259247029 finished with 19 jobs successful and 0 failures, bun x tsc --noEmit exit 0, and 657 focused tests passing across the twelve affected files. An independent review confirmed the stacked tree is byte-identical to the union of the seven PR diffs.

Closing this PR because the change is already on dev. Thank you.

@lidge-jun lidge-jun closed this Sep 8, 2026
@lidge-jun

Copy link
Copy Markdown
Owner

Landed via #4051 at 9587750

@lidge-jun lidge-jun added the landed-via-maintainer Original PR closed after landing via a maintainer merge train label Sep 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working landed-via-maintainer Original PR closed after landing via a maintainer merge train

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants